Notice`s package-private #1197
Notice`s package-private #1197
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request primarily refactors message classes to use package-private visibility for Notice fields, improving encapsulation. It also consolidates sound-playing logic for join, quit, and chat events into their respective controllers, which is a good structural improvement.
However, I've found a critical issue in BroadcastCommand where the executeTitle method has been refactored in a way that breaks its functionality by not replacing placeholders and introducing confusing behavior. I've also spotted a minor typo in the gradlew script. My detailed feedback and suggestions are in the comments below.
| this.noticeService.create() | ||
| .notice(translation -> Notice.title(raw ? " " : translation.broadcast().messageFormat(), text, | ||
| this.settings.titleFadeIn(), this.settings.titleStay(), this.settings.titleFadeOut())) | ||
| .onlinePlayers() | ||
| .send(); |
There was a problem hiding this comment.
The new implementation of executeTitle seems to have a few issues:
- Placeholder Not Replaced: The title is set from
translation.broadcast().messageFormat()when not in raw mode. This format string likely contains placeholders like{BROADCAST}which are not being replaced. This will result in players seeing the literal placeholder string. - Unclear Functionality: The command has been changed to broadcast a "combined title message", and
executeSubtitlehas been removed. However, the implementation always uses the inputtextas the subtitle, and the title is either a space or the unformatted format string. This is a confusing and likely incorrect behavior. It's also a breaking change that isn't documented in the PR description.
If the intention is for this command to now broadcast a subtitle (as executeSubtitle was removed), you could reuse the sendBroadcast helper method which correctly handles placeholder replacement.
Here is a suggested implementation that would make /broadcast title behave like the old /broadcast subtitle:
| this.noticeService.create() | |
| .notice(translation -> Notice.title(raw ? " " : translation.broadcast().messageFormat(), text, | |
| this.settings.titleFadeIn(), this.settings.titleStay(), this.settings.titleFadeOut())) | |
| .onlinePlayers() | |
| .send(); | |
| this.sendBroadcast( | |
| formatted -> Notice.title("", formatted, this.settings.titleFadeIn(), this.settings.titleStay(), this.settings.titleFadeOut()), | |
| text, | |
| raw | |
| ); |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
| this.noticeService.create() | ||
| .notice(translation -> Notice.title(raw ? " " : translation.broadcast().messageFormat(), text, | ||
| this.settings.titleFadeIn(), this.settings.titleStay(), this.settings.titleFadeOut())) | ||
| .onlinePlayers() |
There was a problem hiding this comment.
te zmiany na pewno są do tego PR?
There was a problem hiding this comment.
jeszcze widzę parę usuniętych plików
There was a problem hiding this comment.
oj przez przypadek się połączyły poprawie to ;)
Make Notice`s package-private